-
-
Notifications
You must be signed in to change notification settings - Fork 594
fix(gazelle): Skip indexing py_binary rules if a corresponding py_library rule contains the same srcs #2822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
I don't think that just not indexing This has obvious downsides in that the entrypoint script gets materialized among other things, but it's an existing and working use-case. I think the behavior in this case should simply be to prefer the library target in case both are an option, not ignoring binaries entirely. |
Are you talking about the scenarios when |
Gotcha, I updated the logic to skip indexing the py_binary rule only if there is a corresponding py_library rule that also has the same |
Update update
d37082d
to
75c143d
Compare
@dougthor42 can you also take a look at this PR? One thing I am not sure is whether Gazelle can generate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I am not sure is whether Gazelle can generate py_binary without generating py_library.
In general, yes this is possible. Given
# foo.py
if __name__ == "__main__":
print("hey")
Then, assuming gazelle:python_generation_mode file
, Gazelle will generate:
py_binary(
name = "foo",
srcs = ["foo.py"],
)
However, package
and project
generation modes are different and TBH I'm less familiar with them.
@@ -86,6 +86,7 @@ END_UNRELEASED_TEMPLATE | |||
multiple times. | |||
* (tools/wheelmaker.py) Extras are now preserved in Requires-Dist metadata when using requires_file | |||
to specify the requirements. | |||
* (gazelle): Skip indexing py_binary rules if a corresponding py_library rule contains the same srcs: https://github.com/bazel-contrib/rules_python/pull/2822 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap at ~80chars please
} | ||
pyLibrarySrcs := otherRule.AttrStrings("srcs") | ||
for _, src := range pyLibrarySrcs { | ||
if src == pyBinarySrcs[0] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A py_binary can have multiple srcs
so you don't want to check against only the 1st value.
What are the requirements for not indexing the binary?
- Any binary source file is found in any py_library srcs?
- All binary srcs are found in a single py_library? Eg
binary.srcs
is a subset oflibrary.srcs
. - All binary srcs are found in multiple py_library targets?
Should we make Gazelle always generate |
This PR ensures that py_binary rules are not indexed into Gazelle's IndexMap when there is a corresponding py_library rule with the same srcs. When both py_library and py_binary targets share the same file in their
srcs
, Gazelle previously indexed both under the same import path. This led to ambiguity and resolution errors, as Gazelle found multiple rules for the same language (py).To resolve this, the PR updates Gazelle to skip indexing py_binary rules, allowing py_library to be the only import being indexed.
Testing
An integration test was added where both a py_library and a py_binary rule include the same script.py in their srcs. The test verifies that Gazelle correctly resolves the import to the py_library target.